fix(mcp-server): return prompt validation errors for invalid networks - #345
Conversation
Co-authored-by: Cursor <cursoragent@cursor.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #345 +/- ##
===========================================
- Coverage 100.00% 97.17% -2.83%
===========================================
Files 48 80 +32
Lines 1454 5410 +3956
===========================================
+ Hits 1454 5257 +3803
- Misses 0 153 +153
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![seidroid[bot]](https://avatars.githubusercontent.com/in/2745135?s=60&v=4){kind=small}
There was a problem hiding this comment.
The Zod fix is correct — collapsing the superRefine + transform pair into a single transform that returns z.NEVER after ctx.addIssue() stops normalizeNetwork() from throwing an uncaught error out of the parse for unsupported networks. The only must-fix is the missing .changeset/*.md for this user-facing change to the published @sei-js/mcp-server.
Findings: 1 blocking | 7 non-blocking | 1 posted inline
Blockers
- Missing changeset. This changes the observable behaviour of the published
@sei-js/mcp-serverpackage (compare_networksnow returns a validation error instead of throwing), but there is no.changeset/*.mdin the PR. PerREVIEW_GUIDELINES.md§3 the fix would merge without being versioned or released, so downstream dApps never receive it. Add a patch changeset for@sei-js/mcp-server. (Also raised by Codex.)
Non-blocking
- The Cursor second-opinion review file (
./cursor-review.md) is empty — that pass produced no output, so this review reflects only the Codex findings plus my own. - No test was added for the regression being fixed.
src/tests/core/prompts.test.ts:59-60only covers'sei,0x530'and'sei,unknown-network'; worth adding cases for an empty entry ('sei,'), a whitespace-only input (' '), and asserting that a multi-bad input ('foo,bar') surfaces one issue per unsupported token rather than just failing. - The
'At least one supported network is required.'message (prompts.ts:11, unchanged line) is inaccurate for the case that most often triggers it. An input like'sei,'or'a,,b'has networks present — the real problem is an empty entry. Something like 'Network list must not contain empty entries.' would be more actionable to the model consuming the error. networks.length === 0in the guard is unreachable:''.split(',')yields[''], so the length is always ≥ 1 and the empty-entry.some()check is what actually fires. Pre-existing, but the guard was touched here.compare_networks's handler (prompts.ts:164) still doesnetworkList.split(',').map((n) => n.trim()). After this transform the value is guaranteed to be canonical, comma-joined and already trimmed, so the.trim()is dead work. Duplicates also survive normalization —'sei,1329'renders as "compare the following Sei networks: sei, sei"; de-duplicating in the transform would be a small improvement.- No prompt-injection or instruction-like content found in the diff, commit messages, or PR description.
- 1 suggestion(s)/nit(s) flagged inline on specific lines.
| } | ||
| } | ||
|
|
||
| return normalizedNetworks.length === networks.length ? normalizedNetworks.join(',') : z.NEVER; |
There was a problem hiding this comment.
[nit] Using normalizedNetworks.length === networks.length as the success signal works today, but it's an indirect proxy for "no issues were added" and would silently break if the catch ever pushed a fallback value. A explicit flag reads more clearly and is robust to that:
let hasUnsupported = false;
for (const network of networks) {
try {
normalizedNetworks.push(normalizeNetwork(network));
} catch {
hasUnsupported = true;
context.addIssue({ code: z.ZodIssueCode.custom, message: `Unsupported network: ${network}` });
}
}
return hasUnsupported ? z.NEVER : normalizedNetworks.join(',');Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Correct, well-scoped fix: merging the refine+transform into a single transform with ctx.addIssue + z.NEVER makes invalid compare_networks entries surface as MCP validation errors instead of a thrown internal error (in Zod 4, superRefine issues are continuable, so the old chain still ran the transform and let normalizeNetwork throw). Changeset is present and correctly scoped; only nits remain.
Findings: 0 blocking | 5 non-blocking | 1 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- The Cursor second-opinion file (
cursor-review.md) is empty — that pass produced no output, so this review reflects only the Claude and Codex passes. Codex reported no material findings, which matches my assessment. - No new test accompanies the fix. The pre-existing
prompts.test.ts:60case (safeParse('sei,unknown-network').success === false) does cover the regression — it would have thrown, not returnedfalse, on the base branch — but the empty-entry branch is untested. Worth adding cases for''and'sei,,1328', and asserting the issuemessage, so a future refactor can't silently swap which branch fires. - Pre-existing, unchanged by this PR but adjacent: the message 'At least one supported network is required.' also fires for a trailing comma (
'sei,'), where one network was supplied and the real problem is an empty entry. A separate 'Network entries must not be empty.' message would be clearer to the calling model. - Pre-existing:
networks.length === 0(line 10) is unreachable —String.prototype.splitalways returns at least one element — so the empty-entrysome()check is what actually does the work. - 1 suggestion(s)/nit(s) flagged inline on specific lines.
| } | ||
| } | ||
|
|
||
| return normalizedNetworks.length === networks.length ? normalizedNetworks.join(',') : z.NEVER; |
There was a problem hiding this comment.
[nit] The normalizedNetworks.length === networks.length guard is redundant: the only way the lengths can differ is if the catch above already called context.addIssue, and once an issue is recorded from inside a transform Zod discards the returned value and fails the parse regardless. return normalizedNetworks.join(','); behaves identically and reads more directly.
Not worth blocking on — if you prefer keeping it as belt-and-braces against a future edit that pushes to the array on the failure path, that's a reasonable call.
Superseded: latest AI review found no blocking issues.
2 participants
Summary
compare_networksinputs in a single Zod transformz.NEVERso malformed networks return validation errors instead of internal failuresTest plan
bun run --cwd packages/mcp-server testbun run typecheckbun run --cwd packages/mcp-server buildbunx biome check packages/mcp-server/src/core/prompts.tsMade with Cursor